Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: filter out optimism-goerli-staging from chains #59

Merged
merged 6 commits into from
Oct 25, 2023
Merged

fix: filter out optimism-goerli-staging from chains #59

merged 6 commits into from
Oct 25, 2023

Conversation

dtbuchholz
Copy link
Contributor

@dtbuchholz dtbuchholz commented Oct 24, 2023 •
edited
Loading

Summary

If you try to create a table on OP Goerli, it actually uses the optimism-goerli-staging network instead of optimism-goerli. This filters out the "staging" network and adds tests accordingly. Also, I squeezed in a dep bump for @tableland/sdk. Closes #58.

Details

There's a simple one liner added to helpers/chains.ts:

const filtered = mapped.filter(
  ([chainName]) => chainName !== "optimism-goerli-staging"
);

This is then used in the supportedChains object, which gets exported and is used in clients like the CLI. Note that the CLI had to implement similar logic to what's described above. The change in the SDK will allow us to remove that check in the CLI, but it's not required at the moment; everything should still work as expected in the CLI.

Wrt to the @tableland/sdk@^4.5.3-dev.0 dep bump, I also deleted package-lock.json and reinstalled the overall deps in root. It seems that lerna was having issues with symlinks, and deleting the lock file & updating the packages seemed to fix it. I think lerna might require that each package is using the latest version—e.g., in packages/cli, if I don't use 4.5.3-dev.0 but use any earlier SDK version, I cannot set up a symlink successfully.

How it was tested

Added a test in chains.test.ts to make sure that getChainId("optimism-goerli-staging") will throw an error for cannot use unsupported chain.

I also set up a symlink to the Studio, and I successfully created a table on the correct OP Goerli network.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@dtbuchholz dtbuchholz self-assigned this Oct 24, 2023
@dtbuchholz
Copy link
Contributor Author

@joewagner i made a small adjustment to the SDK to fix a bug that was uncovered during ETHOnline while someone was using the Studio. it removes optimism-goerli-staging from the available chains, and i included some dep chores to resolve lerna symlink issues. lmk if the deps aspect isn't ideal, and i can remove em. cc @asutula

joewagner
joewagner reviewed Oct 24, 2023
View reviewed changes
packages/cli/src/commands/write.ts Outdated
name
);
const { tableId } =
await globalThis.sqlparser.validateTableName(name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the prettier settings are changed. Is this something to do with the deleted package lock?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, looks like it. i dropped that commit, and the formatting went back to normal. i'll just ignore any of the lerna-related things i included here...it might not be the root cause, anyways, after some additional testing...

@joewagner
Copy link
Contributor

joewagner commented Oct 24, 2023 •
edited
Loading

@dtbuchholz this looks good. We might as well put the sdk at an official release number. Looks like something got messed up during the last publish and npm has the sdk's latest version set at 4.5.3-dev.0, which should be a next version.
I think I know how that happened (probably my mistake) but let's fix it with this pr. I think version 4.5.3 makes sense here since it's considered a bug.

asutula
asutula previously approved these changes Oct 24, 2023
View reviewed changes
@dtbuchholz dtbuchholz force-pushed the dtb/fix-op-goerli-staging branch from edacfbf to 9152f4e Compare October 24, 2023 16:29
@dtbuchholz
Copy link
Contributor Author

dtbuchholz commented Oct 24, 2023 •
edited by joewagner
Loading

I think version 4.5.3 makes sense here since it's considered a bug.

@joewagner i bumped the SDK versions in the cli & local packages to 4.5.3. but since it doesn't exist, the tests etc obviously fail here. is that fine, or should i move the dep bump to a separate PR?

edit: i'll also have to bump the package/sdk's package.json version to 4.5.3 too, right? or does the publish GH action somehow handle that? (this change is not included yet, but i'll wait for your input above before i do anything there.)

@dtbuchholz dtbuchholz force-pushed the dtb/fix-op-goerli-staging branch from 9152f4e to 210a5b6 Compare October 24, 2023 20:12
@joewagner
Copy link
Contributor

@joewagner i bumped the SDK versions in the cli & local packages to 4.5.3. but since it doesn't exist, the tests etc obviously fail here. is that fine, or should i move the dep bump to a separate PR?

edit: i'll also have to bump the package/sdk's package.json version to 4.5.3 too, right? or does the publish GH action somehow handle that? (this change is not included yet, but i'll wait for your input above before i do anything there.)

You should be able to set the version to 4.5.3, then update the dependencies in the other packages and as long as the build and test works this PR is good to go. Once merged someone(I can do it, or we can go through it together in voice), can run the lerna publish flow described here: https://www.notion.so/textile/Lerna-Publish-Playbook-55fab12115fd4506926ee1a0556ad63a

@joewagner
Copy link
Contributor

@dtbuchholz I accidentally edited your comment instead of replying...
I tried to put it back how it was, but I'm not totally sure.
I really don't understand why github lets you edit someone else's comment

@dtbuchholz
Copy link
Contributor Author

@dtbuchholz I accidentally edited your comment instead of replying... I tried to put it back how it was, but I'm not totally sure. I really don't understand why github lets you edit someone else's comment

@joewagner no worries...that's such a dumb feature lol

Once merged someone(I can do it, or we can go through it together in voice), can run the lerna publish flow described here: https://www.notion.so/textile/Lerna-Publish-Playbook-55fab12115fd4506926ee1a0556ad63a

i'm down to take a stab! i'll ping you on discord since i know timezones are a bit tough rn

@joewagner joewagner self-requested a review October 25, 2023 16:09
@dtbuchholz dtbuchholz merged commit 1b0871f into main Oct 25, 2023
4 checks passed
@dtbuchholz dtbuchholz deleted the dtb/fix-op-goerli-staging branch October 25, 2023 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joewagner joewagner joewagner approved these changes

@asutula asutula asutula left review comments

@carsonfarmer carsonfarmer Awaiting requested review from carsonfarmer carsonfarmer is a code owner

@sanderpick sanderpick Awaiting requested review from sanderpick sanderpick is a code owner

Assignees

@dtbuchholz dtbuchholz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix: remove optimism-goerli-staging from SDK chains
3 participants
@dtbuchholz @joewagner @asutula